Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 62.2%. Comparing base (722838b) to head (48bd190).
Report is 5 commits behind head on master.

❗ Current head 48bd190 differs from pull request most recent head c9228a9. Consider uploading reports for the commit c9228a9 to get more accurate results

@@          Coverage Diff           @@
##           master   #1280   +/-   ##
======================================
  Coverage    62.2%   62.2%           
======================================
  Files         164     164           
  Lines       12191   12207   +16     
======================================
+ Hits         7584    7599   +15     
- Misses       4607    4608    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Isn't it possible to detect the model automatically (i.e. using the USB VID/PID) instead of requiring manual configuration by the user? How did this work previously? @edersondisouza?

Isn't it possible to detect the model automatically (i.e. using the USB VID/PID) instead of requiring manual configuration by the user?

The user manuals for the YKUSH XS, YKUSH, and YKUSH3 state the products have the same VID but different PIDs. Therefore, I believe it is possible to detect the model automatically. Would you advise changing YKUSHPowerPort to inherit from USBResource?

How did this work previously?

pykush does not have a board_type argument.

$ python3.8 pykush.py -l
listing YKUSH family devices
  found a YKUSH XS release 2 device with serial number YKU2768
    system device path 1-8.2:1.0, vendor id 0x04d8, product id 0xf0cd
    the device has 1 downstream port
    Checking running power state, port 1 : UP
  found a YKUSH3 release 1 device with serial number Y3N10673
    system device path 1-4.4:1.0, vendor id 0x04d8, product id 0xf11b
    the device is running a v1.0 firmware and has 3 downstream ports
    downstream running power states, port 1 to 3: UP, UP, UP

ykushcmd requires the board_name argument for the YKUSH XS and YKUSH3 boards.

$ ./bin/ykushcmd -l
Attached YKUSH Boards:
No YKUSH boards found.

$ ./bin/ykushcmd ykush3 -l
Attached YKUSH3 Boards:
1. Board found with serial number: Y3N10673

$ ./bin/ykushcmd ykushxs -l
Attached YKUSH XS Boards:
1. Board found with serial number: YKU2768

Ouch, this is really inconvenient =/

Wonder if trying to get USB PID will work nice with the remote instance. Maybe require pykush and use the command line tool (not the module, so it's easy to support remote instance)?

An ugly hack would be to try to chain three calls to ykushcmd and use it, like ykushcmd ... | ykushcmd ykush3 ... | ykushcmd ykushxs... (/me hides in the corner).

Or fix ykushcmd itself... =/

Fixing ykushkmd would be nice and should be easy based on the VID/PID info.

Otherwise, you could make YKUSHPowerPort inherit from USBResource, which will give it additional USB properties (busnum, devnum, path, vendor_id and model_id). NetworkYKUSHPowerPort would then inherit from RemoteUSBResource to store these new properties.

YKUSHPowerPort would match USB HID devices by the given serial. In the Driver, you could then use the USB vendor/model IDs to generate the correct board name for ykushcmd.

The additional resource properties would break backwards compatibility, but the current state is broken anyway. :/

The additional resource properties would break backwards compatibility, but the current state is broken anyway. :/

I found a way to not break backward compatibility. Use ykushcmd to list out the attached boards by model. Then determine the model of a given board by looking for the serial number in the output for each model. It will require adding each new board to the YKUSHPowerDriver but the PID/VID solution had the same issue.

Nice workaround! Minor nit: on commit message the phrase "Added tests for YKUSHPowerPort and YKUSHPowerPort.' seems wrong - maybe you meant YKUSHPowerDriver for the second YKUSHPowerPort?

Yes, thanks for catching that.

Any more comments on this, @jluebbe and @Bastian-Krause?